🔍 Rust PR Review

Summary: Looks good — correctness solid, tests comprehensive. A few minor points worth addressing.

Findings

⚠️ Suggestions

• src/compile/common.rs:1636-1642 — The warning-and-skip path for unknown safe-output tool names in generate_enabled_tools_args is now dead code on the normal compile path. validate_safe_outputs_keys runs at line 3000 (validation phase) and bails hard before generate_enabled_tools_args can ever see an unknown key. The comment at line 1657 ("Every user-specified key was either invalid or unrecognized") is no longer accurate for the unknown case. Since generate_enabled_tools_args is pub, the defensive check isn't harmful, but it creates a misleading impression that warnings are the error surface here. Consider adding a comment like // unreachable in practice: validate_safe_outputs_keys bails earlier or removing the unrecognized-key branch now that the validator owns that responsibility.

• src/compile/common.rs:1904-1910 — validate_safe_outputs_keys bails on the first invalid-character key but collects all unknown-but-syntactically-valid keys before reporting. If a user has two bad-character keys they'll only see the first. Minor UX inconsistency — consider collecting character-invalid keys into the same unknown vec (with a different message) so the report is complete in one pass.

• src/compile/common.rs:1964 — key.split('-').next().unwrap_or(key) — the .unwrap_or(key) fallback is unreachable: str::split().next() always returns Some(...) (even for "", it yields Some("")). The empty-string case is already handled by the if head.is_empty() guard below. The unwrap_or is harmless but a reader might wonder why it's there. unwrap_or_default() or an inline comment would be cleaner.

✅ What Looks Good

• The validate_safe_outputs_keys / validate_ado_aw_debug_config split is well-designed: debug-only tools get a purpose-specific error, unknowns get the typo-hint error, memory gets the soft migration warning. The ordering (line 3000 then 3006) makes the sequencing explicit.
• The reply-to-pr-comment rename is thorough — all five sites updated (mcp.rs #[tool(name=...)], execute.rs dispatch, tool_result! macro, log strings, four unit test assertions).
• 9 unit tests cover all meaningful branches of validate_safe_outputs_keys and related_safe_output_names, including the dedup-for-debug-only and soft-migration cases.
• Rejecting create-pr-style typos with a create-* candidate list (rather than a Levenshtein nearest-neighbor) is the right call — the PR description's reasoning about update-pr false-matches is solid.

Generated by Rust PR Reviewer for issue #563 · ● 1.1M · ◷

@copilot implement suggestions

@copilot implement suggestions

Done in 90467d7. All three suggestions from the automated review are now addressed:

1. Dead branch in generate_enabled_tools_args — replaced the eprintln warning with a // Unreachable in practice: validate_safe_outputs_keys bails before… comment; updated the stale effective_mcp_tool_count == 0 comment too.
2. Collect all invalid-character keys — validate_safe_outputs_keys now collects every key that fails is_safe_tool_name into invalid_names and reports them all in one error, rather than bailing on the first.
3. unwrap_or(key) → unwrap_or_default() — str::split().next() never returns None, so the fallback was unreachable; unwrap_or_default() (yields "") is cleaner and the existing head.is_empty() guard handles that case.

A new unit test test_validate_safe_outputs_keys_reports_all_invalid_characters covers the multi-key path.

/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Looks good — clean implementation with solid test coverage; one minor log-string inconsistency noted.

Findings

⚠️ Suggestions

• src/mcp.rs:714 — Log message still says "Tool called: create_pr" while the parallel rename for reply-to-pr-comment correctly updated its log string to the wire name ("Tool called: reply-to-pr-comment"). Inconsistency only; no behavioral impact.

  // current
  info!("Tool called: create_pr - '{}'", params.0.title);
  // consistent with the rename pattern:
  info!("Tool called: create-pull-request - '{}'", params.0.title);

✅ What Looks Good

• validate_safe_outputs_keys design is sound. The special-case ordering (invalid_names → NON_MCP_SAFE_OUTPUT_KEYS → memory → DEBUG_ONLY_TOOLS → ALL_KNOWN_SAFE_OUTPUTS) matches the comments exactly and avoids double-reporting the create-issue debug-only tool. Early bail on invalid characters is the right call.

• related_safe_output_names over Levenshtein — the PR description explains the reasoning well, and the first-hyphen-segment grouping is provably correct: it lists every create-* candidate without the false hit of update-pr for create-pr. The test that verifies this exclusion (!related.contains(&"update-pr")) is a nice explicit guard.

• reply-to-pr-review-comment → reply-to-pr-comment rename is complete — updated across all four call sites: tool_result! macro declaration, mcp.rs #[tool(name = ...)], execute.rs dispatch match arm, get_tool_config lookup key, and all four unit tests. No leftover references found in src/, docs/, prompts/, or tests/.

• create_pr.rs → create_pull_request.rs module rename is surgical: the file name moves to match the wire key, struct names (CreatePrResult, CreatePrParams, CreatePrConfig) are preserved (avoiding churn), and CreatePrResult::NAME == "create-pull-request" was already correct before this PR via the tool_result! macro. ALL_KNOWN_SAFE_OUTPUTS derives names from the type's NAME constant so the constant list required zero updates.

• Validation is wired into all compile targets — validate_safe_outputs_keys is called inside compile_shared, which is reachable from standalone, 1ES, job, and stage paths.

• 9 unit tests cover the full decision tree including the memory migration soft-path, the create-issue debug-only bypass, the all-invalid-keys-in-one-error accumulation, and the related_safe_output_names prefix logic.

Generated by Rust PR Reviewer for issue #563 · ● 1.5M · ◷

@copilot implement suggestion

@copilot implement suggestion

Fixed in 9145e4f — "Tool called: create_pr" → "Tool called: create-pull-request" to match the wire name convention used by the other tools.